Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

improve handling of file URIs #5892

Merged
merged 2 commits into from
Feb 24, 2019
Merged

improve handling of file URIs #5892

merged 2 commits into from
Feb 24, 2019

Conversation

benoit-pierre
Copy link
Member

  • correctly handle file://localhost/... URIs, as per RFC 8089: file://localhost/some/path is equivalent to file:///some/path
  • don't try to use UNC paths on Unix

@benoit-pierre benoit-pierre force-pushed the improve_handling_of_file_uris branch from 1af2bfe to c7ef9d9 Compare October 17, 2018 12:17
@benoit-pierre benoit-pierre mentioned this pull request Oct 17, 2018
Closed
@benoit-pierre benoit-pierre force-pushed the improve_handling_of_file_uris branch from c7ef9d9 to fe7584b Compare October 18, 2018 21:30
cjerdonek
cjerdonek reviewed Oct 30, 2018
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some review comments.

Also, can the problem this PR is fixing be seen from the CLI? I think it would probably be good to have at least one higher-level regression test, especially since this function is a core function used throughout the code base. For that reason, I'm surprised it doesn't cause more problems. What must one do to hit this issue in real life?

tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated
assert url_to_path('file://unc/as/path') == r'\\unc\as\path'
assert url_to_path('file:c:/path/to/file') == r'C:\path\to\file'
assert url_to_path('file://localhost/c:/tmp/file') == r'C:\tmp\file'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other ordering comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it would be nice if the test cases that are supposed to run on all platforms are broken out into a third test. That way there will be less repetition of test cases, and it's easier to see which cases are the same on both platforms.

@pradyunsg pradyunsg added type: enhancement Improvements to functionality C: download About fetching data from PyPI and other sources S: awaiting response Waiting for a response/more information labels Nov 10, 2018
@benoit-pierre benoit-pierre mentioned this pull request Dec 13, 2018
Closed
@xavfernandez xavfernandez reopened this Feb 11, 2019
@xavfernandez
Copy link
Member

@cjerdonek I think the change seems sound and the tests could be improved in an other PR.

@cjerdonek
Copy link
Member

My preference would be for the review comments I made to be addressed with the PR.

@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch from fe7584b to 5cd50d2 Compare February 11, 2019 21:37
cjerdonek
cjerdonek reviewed Feb 11, 2019
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments.

tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch 3 times, most recently from b1e2ed0 to bb5c83f Compare February 11, 2019 22:14
@xavfernandez
Copy link
Member

@cjerdonek I think everything should now be fine ?

@cjerdonek
Copy link
Member

Okay, thanks, @xavfernandez. I appreciate it. Now that I can see the tests more clearly, I have one more comment / question.

For each file URI that appears in our test cases, shouldn't we have a test for it on both Windows and non-Windows? The reason I ask is that it doesn't seem like we can guarantee that a given file URI will only be seen on a given platform, so it seems like pip should be resilient in the face of such input, or at least we should have an understanding of / be able to see how it behaves in that case (even if that behavior is to error out). In other words, I agree the test expectation should be platform-dependent, but it seems like the test inputs should be run on both platforms. One way to do that would be to have a single test with parameters (url, win_expected, non_win_expected), perhaps with a special value to signal ValueError. There are other ways, of course, but that's one way to make it easy to confirm that each url is being run on both platforms and will continue to be going forward.

@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch 2 times, most recently from b079a47 to b482b4b Compare February 12, 2019 16:25
@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch from bfdfdd6 to 8556b7f Compare February 12, 2019 17:45
@xavfernandez
Copy link
Member

ping @cjerdonek :)

@cjerdonek
Copy link
Member

cjerdonek commented Feb 12, 2019 •
edited
Loading

Thanks, looks great!

By the way, I was just pinged about #6247 a couple hours ago. Do you know how this PR will affect that issue -- it looks like this means we won't be accepting on Unix systems what that poster is calling a relative file URI (will raise ValueError judging by the test cases)?

cjerdonek
cjerdonek approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving assuming you're okay with the implications for #6247. Thanks for picking this up, @xavfernandez!

@cjerdonek cjerdonek mentioned this pull request Feb 12, 2019
Closed
@cjerdonek
Copy link
Member

For future reference, this seems related to this issue #783.

Out of curiosity, does this PR solve an actual end-user issue, or is it more a matter of purity? The reason I ask is that I don't see an issue number referenced or description of a concrete use case. For example, even #783's latest comment says that it "seems to indicate that this is working now."

@cjerdonek
Copy link
Member

(Btw, you should probably wait until after 19.0.3 is released before merging.)

@cjerdonek cjerdonek removed the S: awaiting response Waiting for a response/more information label Feb 13, 2019
@xavfernandez
Copy link
Member

@cjerdonek it was more a matter of purity :)

But FWIW it means that:

$ pip install file://localhost/tmp/some_wheel-6.5-py2.py3-none-any.whl --no-index
Processing ./\\localhost/tmp/some_wheel-6.5-py2.py3-none-any.whl
Could not install packages due to an EnvironmentError: [Errno 2] No such file or directory: '/tmp/\\\\localhost//tmp/some_wheel-6.5-py2.py3-none-any.whl'`

would now work :)

@xavfernandez xavfernandez merged commit c286c55 into pypa:master Feb 24, 2019
@xavfernandez
Copy link
Member

Thanks @benoit-pierre for the initial PR and thanks @cjerdonek for the thorough review :)

@benoit-pierre benoit-pierre deleted the improve_handling_of_file_uris branch February 24, 2019 20:54
@pradyunsg
Copy link
Member

Thanks everyone who worked on this!

opencontrail-ci-admin pushed a commit to Juniper/contrail-controller that referenced this pull request May 16, 2019
@pchoudhary-jnpr
Fixing Opserver UT
51731b8 
Currently Opserver UT is failing in R3.2 branch because of following issues.
1. In latest release of pip, url_to_path handling has changed via pypa/pip#5892.
   This is breaking Opserver UT in R3.2. But It works in latest master.
   Fix - Make R3.2 opserver/Sconsript same as master.

2. Cassandra 2.1.9 is not spawning by mockcassandra.
   Fix - This cassandra version is affected by https://issues.apache.org/jira/browse/CASSANDRA-11661.
   Upgrade cassandra version to 2.2.12. This is same as cassandra version running on 3.2.13 deployments.
   Mockcassandra is using pycassa to connect to cassandra on thrift port. So, set start_rpc TRUE in cassandra yaml.

Change-Id: I0d62d6a6487f284cc56bb904da6d5ba5ba191833
closes-jira-bug: CEM-5615
@lock
Copy link

lock bot commented May 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez approved these changes

@cjerdonek cjerdonek cjerdonek approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: download About fetching data from PyPI and other sources type: enhancement Improvements to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@benoit-pierre @xavfernandez @cjerdonek @pradyunsg